Allow Command-Escape keybindings - #4307
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR introduces native/global shortcut registration, cross-process keyboard routing, and broad changes to Escape dismissal behavior across shared UI primitives and production components. Its cross-platform lifecycle and behavioral scope warrant human review despite targeted test coverage. You can add or adjust custom eligibility rules. Learn more. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9d2d6ae96
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
70f663e to
8d85b2c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d85b2c618
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
8d85b2c to
12c5b19
Compare
d25021f to
d83a665
Compare
…flict-comment-fixes # Conflicts: # apps/desktop/src/window/DesktopWindow.ts # apps/web/src/components/CommandPalette.tsx # apps/web/src/routes/settings.tsx
There was a problem hiding this comment.
One consistency issue found in the keybinding recorder: it now accepts Escape combinations that isEscapeDismissal still classifies as ordinary dismissal, so those bindings collide with the app-wide Escape handling this PR just standardized. Details inline.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One finding: the new isEscapeDismissal contract is only applied to about half of the app's Escape-dismissal handlers, so mod+esc still double-fires at the remaining sites in browser mode. Details inline.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One finding: the project-script keybinding recorder is now part of the native mod+esc capture contract but does not follow the isEscapeDismissal policy the two settings recorders adopted, so it can still record Escape combos that the rest of the app treats as dismissals.
The previously flagged issues (incomplete isEscapeDismissal adoption across web Escape handlers, and the settings recorders accepting non-mod Escape combos) are addressed at this head.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One consistency gap on the new mod+esc reservation: Base UI's own Escape dismissal paths are still ungated except in projectScriptEditor.tsx, so in the browser mod+esc continues to dismiss overlays and the new gate in CommandPalette has no effect. Details inline.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One finding: the new mod+esc dismissal gate added to the model picker is attached to the inline Combobox, which never emits an escape-key open-change, while the Popover that actually owns the picker's visibility stays ungated. Details inline.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One finding: the new mod+esc reservation is installed per call site on only three Base UI overlays, so identical overlays elsewhere still dismiss on mod+esc. Detail inline on apps/web/src/components/chat/ProviderModelPicker.tsx.
The raw event.key === "Escape" migration itself looks complete now — no ungated "Escape" comparisons remain under apps/web/src, and both keybinding recorders plus projectScriptEditor now cancel recording through isEscapeDismissal, so non-mod Escape chords are no longer recordable while being treated as dismissals elsewhere.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Two gaps in the new mod+esc reservation, both in the shared overlay layer.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One finding: the new macOS Escape shortcut wiring in apps/desktop/src/window/DesktopWindow.ts registers app-level Electron listeners through the raw Electron.app global instead of the ElectronApp service the module already acquires from the environment.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
One finding: the new macOS Escape shortcut wiring in apps/desktop/src/window/DesktopWindow.ts registers app-level Electron listeners through the raw Electron.app global instead of the ElectronApp service the module already acquires from the environment.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9ce96d8. Configure here.
| { | ||
| accelerator: "Command+Alt+Escape", | ||
| input: { key: "Escape", metaKey: true, ctrlKey: false, altKey: true, shiftKey: false }, | ||
| }, |
There was a problem hiding this comment.
Force Quit shortcut stolen on macOS
High Severity
MACOS_MOD_ESCAPE_SHORTCUTS registers Command+Alt+Escape with globalShortcut whenever T3 Code is focused. That chord is macOS Force Quit, and Electron can intercept it, so Force Quit is suppressed exactly while this app has focus—the case where users need it most if the UI hangs.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9ce96d8. Configure here.
| <Label htmlFor="script-keybinding">Keybinding</Label> | ||
| <Input | ||
| id="script-keybinding" | ||
| data-keybinding-capture="" |
There was a problem hiding this comment.
Recorder bypassed by ChatView capture
Medium Severity
Native mod+esc is dispatched to the project-script input marked data-keybinding-capture, but ChatView’s capture-phase shortcut handler never skips that attribute (unlike AppSidebarLayout). From the chat header script editor, an already-bound mod+esc can run via resolveShortcutCommand before the recorder sees the event.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9ce96d8. Configure here.
|
Closing in favor of a fresh, substantially simplified replacement PR. |


Summary
Screen.Recording.2026-07-22.at.4.30.09.PM.mov
mod+escwhile keeping bare Escape as the recorder cancel actionRoot cause
macOS consumes a physical Command-Escape chord before Chromium emits the renderer keyboard event. The keybinding recorder therefore stayed blank even though its web-level normalization supported
mod+esc.Validation
vp test run apps/web/src/components/settings/KeybindingsSettings.logic.test.ts apps/desktop/src/keybindings/NativeKeybindingCapture.test.ts apps/desktop/src/preview/Manager.test.ts apps/desktop/src/window/DesktopWindow.test.ts(56 tests)vp lintfor all changed filesvp run typecheckinapps/desktopvp run typecheckinapps/webmod+escin T3 Code DevNote
Allow
mod+esckeybindings and prevent modifiedEscapefrom dismissing UIisEscapeDismissalandcancelNonDismissalEscapein keybindings.ts to distinguish plainEscapefrom modifiedEscape(e.g.,Cmd+Esc)Escape, allowing modifiedEscapeto propagateEscapeat the Electron level and forward it to the web app for keybinding captureEscapewithmetaKey(macOS) orctrlKey(other platforms) no longer closes dialogs, menus, or settings panels; it is now captured as a keybinding. Any component not updated to useisEscapeDismissalwill still treat modifiedEscapeas dismissalMacroscope summarized 9ce96d8.
Note
Medium Risk
Touches global shortcuts and broad Escape handling across the app; incorrect dismissal logic could leave dialogs open or break cancel flows, but changes are covered by targeted tests.
Overview
macOS never delivers Command+Escape to the renderer, so this PR captures it in Electron and re-injects a synthetic
keydownso shortcuts can be recorded and used asmod+esc.On macOS, the main window registers the full Command+modifier+Escape set via
globalShortcutwhile the app is focused (unregister on blur/close). Preview webviews and non-macOS main windows usebefore-input-eventtopreventDefaultand IPCdesktop:native-keybinding-capturethrough preload into the renderer. Capture targetsdata-keybinding-capturewhen that input is focused, otherwisewindow.The web layer adds
isEscapeDismissalandcancelNonDismissalEscapeso plain Escape still cancels UI, but platform-mod+Escape is not treated as dismiss. That logic is wired through keybinding recorders, many dismiss handlers, and Base UI roots (dialog, menu, combobox, etc.).keybindingFromKeyboardEventalready maps modified Escape tomod+esc; bare Escape staysnullfor cancel during capture.Reviewed by Cursor Bugbot for commit 9ce96d8. Bugbot is set up for automated code reviews on this repo. Configure here.